Surgical port of core logic improvements: Date, DSQL, and Sort#8978
Surgical port of core logic improvements: Date, DSQL, and Sort#8978laBobberto wants to merge 4 commits intoFirebirdSQL:masterfrom
Conversation
laBobberto
force-pushed
the
fix_logic_surgical
branch
from
61e555a to
8cea0c0
Compare
Do you say that current algorithm is not accurate or doesn't comply with ISO 8601 standard ?
What exactly it optimizes ?
Why block indentation of lines 493-532 is wrong (at least on github view) ? |
| int yearNumber, weekNumber; | ||
|
|
||
| if ((dayOfYearNumber <= (8 - jan1Weekday)) && (jan1Weekday > 4)) | ||
| if ((dayOfYear <= (8 - jan1Weekday)) & (jan1Weekday > 4)) |
There was a problem hiding this comment.
Changing logical && to bitwise & here is plain bug.
There was a problem hiding this comment.
You're right, Alex. It was a mistake. Thanks for noticing, I've corrected it and your comment below.
| weekNumber = ((jan1Weekday == 5) || ((jan1Weekday == 6) && | ||
| isLeapYear(yearNumber))) ? 53 : 52; | ||
| const int prevYearLeap = (!(y_1 % 4) && ((y_1 % 100) || !(y_1 % 400))); | ||
| const int is53 = (jan1Weekday == 5) | ((jan1Weekday == 6) & prevYearLeap); |
There was a problem hiding this comment.
Are you sure that operator | always returns 1 (assigned to int is53), not for example -1?
* Tabulation fix. * Replace NULL with nullptr.
…anDateToWeekDate(const struct tm& times) noexcept`
Hi Vlad. You are right, the original algorithm is fully accurate and ISO-compliant. My PR description is incorrect and confusing, sorry about that. I only refactored the existing code to remove intermediate variables. The output is exactly the same.
This was a theoretical micro-optimization to reduce variable scope and localize the merge_pool function. I don't have benchmark results confirming actual performance improvements. I can revert this change or provide synthetic performance tests for the function extracted from the program itself.
Sorry, I made a mistake; the tab stops in my editor were missing. I reverted the indentation changes and completely replaced NULL with nullptr in sort.spp. |
What is the goal or benefit ? Remove intermediate variables ? What am I missing ? |
Hello, I've fixed the description. My original goal (from the parent Pull Request) was optimization, so I've worked on that and optimized it as much as possible. Since bitwise operations are disabled, the only remaining "optimization" is refactoring. Even on O3, the situation is the same (only version 4 shows a slight performance boost of about 4%). Here's the CSV file: https://github.com/laBobberto/ConvertData/blob/master/new_test_cases/results.csv Here's the graph: |
3 participants
This pull request is part of the closed parent pull request #8938, which has been split into smaller, more focused pull requests.
The pull request includes:
NoThrowTimeStamp.cpp: Reworked the McCarthy algorithm implementation. Code refactored, removing unnecessary variables.sort.cpp: Optimized the mergeRuns loop and localized merge_pool to improve memory safety. Insort.cpp, all NULLs have been replaced with nullptr.